-
Notifications
You must be signed in to change notification settings - Fork 55
Added retry logic to improve state change success #43
base: master
Are you sure you want to change the base?
Conversation
I'll test it tonight. But the problem I see now is, you always catch the exception with continue. That means the exception might always get ignored. Will this be better? for i in range(3):
try:
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
s.settimeout(self.connection_timeout)
s.connect((self.address, self.port))
s.send(payload)
data = s.recv(1024)
s.close()
return data
except socket.error:
if i >= 3:
# raise some exception
else:
continue |
Good point, nice catch! I'll make that suggested edit now. |
@left4taco Updated to handle like so
|
Sure. Will get back to you after I tested it. Thanks.
…On Mon, Oct 29, 2018 at 11:57 AM Michael Hill ***@***.***> wrote:
@left4taco <https://github.com/left4taco> Updated to handle like so
except socket.error as e:
if i == 2:
raise ConnectionError(e)
else:
continue
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEl9xzodOxYXLVuCvjH7PKvye9Tm7FSeks5up0-WgaJpZM4X_0Xz>
.
|
The problem I found with this modification is, getting status too frequently will toggle the switch randomly. This is also mentioned in some ticket but I cannot find it. |
Hmm, that's peculiar behavior. I would think with the retry logic it should only fetch upon errors. Any suggestions? I mainly need this retry logic for actually setting the status since it sometimes fails with the current setup. |
As implemented, if you get a receive error you will resend the command.
You need to break up the retry code around section you want to retry and
ones you do not...
…On Tue, Oct 30, 2018 at 4:05 AM Michael Hill ***@***.***> wrote:
Hmm, that's peculiar behavior. I would think with the retry logic it
should only fetch upon errors.
Any suggestions? I mainly need this retry logic for actually setting the
status since it sometimes fails with the current setup.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#43 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADLj6BmEGCMljydLxXDV9998ZIE8Cqs-ks5uqDKIgaJpZM4X_0Xz>
.
|
@BillSobel retry logic moved to |
No problem. Will test when I get home. |
The changes look good to me. |
Any word on this? Curious if @BillSobel has had a chance to test this and if he noticed any negative side-effects or if it did in fact improve successful state-change rate. |
I’m not using this code so I’ll never test it. I was the author of the c# version and the start linking ported here (hence why I’m on the list)
Sent from my iPhone please excuse any typos.
… On Nov 5, 2018, at 8:26 AM, Michael Hill ***@***.***> wrote:
Any word on this? Curious if @BillSobel has had a chance to test this and if he noticed any negative side-effects or if it did in fact improve successful state-change rate.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Ahh, my mistake, sorry! In that case, guess I'm looking for @clach04's input then. |
Sorry for delay in responding. Code looks good BUT for re-try logic I'd like to see it parameterized rather than hard coded (in this instance to 3). Related bug where status calls causes state toggle is upstream issue is #84 (local issue #29) - I've yet to see this but I totally believe this is happening with some devices. |
@clach04 No worries, I understand life gets busy. I agree that parameterizing the retry logic would be cleaner. How would you propose I do so though? Do you want me to implement an additional (optional) parameter to the
Also, I see that you mention there is an upstream issue related to this. Does that mean we shouldn't be implementing this fix here or is that still ok? |
I recently found it quite unstable. So I have to move the retry logic into |
Closes #31
❗ Important ❗ I need others to test this change to see if it works as intended and to ensure it doesn't pose any unintended side-effects (memory leaks, etc).